Host: Monitor and serve important contract addresses #1645
Conversation
## Walkthrough
The changes across various files indicate a significant update to the management of important contracts within an Ethereum-based network, likely Obscuro. The `L1Publisher` interface and related components have been updated to handle transactions relevant to these contracts, including new methods for syncing and retrieving contract addresses. The `MgmtContractLib` interface has been expanded with additional methods for managing contract keys and addresses. Integration tests and network simulation components have been updated to support these changes, ensuring that important contracts can be set and verified within the network.
## Changes
| File Path | Change Summary |
|-----------|----------------|
| `go/common/host/services.go`<br>`go/host/l1/publisher.go` | Updated `L1Publisher` interface with new methods for handling important contracts and extracting relevant transactions. Added mutex and map for caching contract addresses. |
| `go/common/query_types.go`<br>`go/ethadapter/l1_transaction.go` | Added new field `ImportantContracts` to `ObscuroNetworkInfo` struct and introduced `L1SetImportantContractsTx` type. |
| `go/ethadapter/mgmtcontractlib/...`<br>`integration/ethereummock/mgmt_contract_lib.go` | Expanded `MgmtContractLib` interface with new methods for managing important contract keys and addresses. Renamed and added functions to handle host and important contract addresses. |
| `integration/networktest/actions/l1/important_contracts.go`<br>`integration/networktest/tests/bridge/important_contracts_test.go` | Added new package `l1` with `setImportantContract` type and `SetImportantContract` function for setting and verifying important contracts in tests. |
| `integration/networktest/env/testnet.go`<br>`integration/simulation/devnetwork/dev_network.go` | Added `GetMCOwnerWallet` method to network connectors for retrieving the management contract owner wallet. |
| `integration/networktest/interfaces.go` | Added `GetMCOwnerWallet()` method to the `NetworkConnector` interface and updated comments. |TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- contracts/generated/ManagementContract/ManagementContract.go
Files selected for processing (9)
- go/common/host/services.go (2 hunks)
- go/common/query_types.go (1 hunks)
- go/ethadapter/l1_transaction.go (1 hunks)
- go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (1 hunks)
- go/ethadapter/mgmtcontractlib/mgmt_contract_lib.go (4 hunks)
- go/host/enclave/guardian.go (3 hunks)
- go/host/host.go (1 hunks)
- go/host/l1/publisher.go (6 hunks)
- integration/ethereummock/mgmt_contract_lib.go (1 hunks)
Additional comments: 14
go/common/query_types.go (1)
- 89-92: The addition of the
ImportantContractsfield to theObscuroNetworkInfostruct is a significant change. It is important to ensure that all parts of the code that instantiate or use this struct are updated to handle this new field. This includes serialization/deserialization logic, database schema updates if this struct is persisted, and any API changes that might expose this struct to external systems.Additionally, consider initializing this map in the struct's constructor or wherever the
ObscuroNetworkInfostruct is instantiated to avoid nil map errors when trying to write to it.go/common/host/services.go (2)
101-108: The
ExtractObscuroRelevantTransactionsmethod has been updated to return three slices of different transaction types. Ensure that all implementations of theL1Publisherinterface and usages of this method have been updated to handle the new return types correctly. This change could potentially break existing code that expects the previous method signature.116-119: The addition of
GetImportantContractsandResyncImportantContractsmethods to theL1Publisherinterface is a significant change. Ensure that all implementations of this interface are updated to include these new methods. Additionally, verify that the logic for caching and resyncing important contracts is implemented correctly and that it handles errors and edge cases appropriately.go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (1)
- 3-16: The changes to the constants in the
mgmtcontractlibpackage reflect the renaming and addition of methods related to important contract addresses. It's important to ensure that these constant names accurately represent the corresponding method names in the smart contract and that they are used consistently throughout the codebase.
- The renaming of
GetHostAddressesMethodtoGetHostAddressesMsgshould be verified across all files to ensure that any code referencing the old name is updated.- The addition of
GetImportantContractKeysMethod,SetImportantContractsMethod, andGetImportantAddressMethodshould be cross-checked with the actual contract to ensure that the method names match.- The comment
//#nosecon line 9 suggests that a security linter is being instructed to ignore the next line. Verify that this is intentional and that the security implications are fully understood and accepted.integration/ethereummock/mgmt_contract_lib.go (1)
- 98-98: The
panicin thedecodeTxfunction is used to handle unexpected conditions, such as an empty transaction data or an unexpected transaction type. While this is acceptable in a mock or test environment, in production code, it would be better to handle these errors gracefully by returning them to the caller, allowing the caller to decide how to handle the error. However, since this is a mock implementation, the use ofpanicmay be justified if it's meant to signal a test failure in case of incorrect usage.go/ethadapter/l1_transaction.go (2)
33-34: The
L1RespondSecretTxstruct seems to have a new fieldHostAddressadded. This change should be verified to ensure that all parts of the code that instantiate or use this struct are updated accordingly to handle the new field. This includes serialization and deserialization logic, as well as any functions that construct or processL1RespondSecretTxobjects.36-39: The introduction of the
L1SetImportantContractsTxstruct is a significant change. It is important to ensure that the rest of the system is aware of this new transaction type and that it is handled appropriately wherever transactions are processed. This includes updating any switch statements or type assertions that deal withL1Transactiontypes, as well as ensuring that any new logic required to process this transaction type is implemented.go/host/enclave/guardian.go (1)
- 455-469: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [435-467]
The asynchronous call to
ResyncImportantContractsis a good addition for handling updates to important contract addresses. However, it's important to ensure that this does not lead to race conditions or inconsistencies if multiplecontractAddressTxsare processed in quick succession. Consider adding a mechanism to prevent concurrent resync operations or to handle them in a way that ensures data consistency.go/host/l1/publisher.go (6)
4-9: The import of
syncis necessary for the mutex used in thePublisherstruct. The import oftimeis also necessary for themaxWaitForL1ReceiptandretryIntervalForL1Receiptfields in thePublisherstruct. These imports are correctly placed and used.28-38: The addition of the
importantContractAddressesmap and theimportantAddressesMutexto thePublisherstruct is a good practice for thread-safe access to shared resources. This ensures that the cache of important contract addresses can be safely accessed and modified by multiple goroutines.63-69: The initialization of the
importantContractAddressesmap and theimportantAddressesMutexin theNewL1Publisherconstructor is correct and ensures that the map is not nil and the mutex is ready for use.161-188: The
ExtractObscuroRelevantTransactionsmethod has been correctly updated to extractL1SetImportantContractsTxtransactions in addition to the other transaction types. This change aligns with the pull request summary and is necessary for the new functionality.224-237: The
FetchLatestPeersListmethod correctly fetches and decodes the list of host addresses. However, the code to filter out duplicates and the publisher's own address is not shown in the hunk. Ensure that the logic for filtering is correctly implemented and tested.252-302: The
GetImportantContractsmethod correctly uses a read lock to safely return the current state of theimportantContractAddressesmap. TheResyncImportantContractsmethod is responsible for updating the cache of important contract addresses. It correctly locks the map for writing when updating it. This is a good practice to prevent data races.
| func (m *mockContractLib) GetHostAddressesMsg() (ethereum.CallMsg, error) { | ||
| return ethereum.CallMsg{}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) DecodeCallResponse([]byte) ([][]string, error) { | ||
| return [][]string{{""}}, nil | ||
| func (m *mockContractLib) DecodeHostAddressesResponse([]byte) ([]string, error) { | ||
| return []string{""}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) GetImportantContractKeysMsg() (ethereum.CallMsg, error) { | ||
| return ethereum.CallMsg{}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) DecodeImportantContractKeysResponse([]byte) ([]string, error) { | ||
| return []string{""}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) GetImportantAddressCallMsg(string) (ethereum.CallMsg, error) { | ||
| return ethereum.CallMsg{}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) DecodeImportantAddressResponse([]byte) (gethcommon.Address, error) { | ||
| return gethcommon.Address{}, nil | ||
| } |
There was a problem hiding this comment.
The mock implementations for the new RPC methods (GetHostAddressesMsg, DecodeHostAddressesResponse, GetImportantContractKeysMsg, DecodeImportantContractKeysResponse, GetImportantAddressCallMsg, DecodeImportantAddressResponse) return empty or default values. This is fine for placeholder implementations, but if these methods are expected to be used in tests, they should be implemented to return meaningful mock data. Otherwise, tests using these methods may not be effective.
For example, DecodeHostAddressesResponse and DecodeImportantContractKeysResponse could return a list of mock addresses or keys, and DecodeImportantAddressResponse could return a mock address based on some input criteria or random generation.
If these methods are not yet implemented because they are pending further development, consider adding a TODO comment or throwing a not implemented error to make it clear that these are stubs.
| func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx { | ||
| var data []byte |
There was a problem hiding this comment.
The Sign function for L1RespondSecretTx is provided, but it seems to be incomplete. The data variable is declared but not initialized with the Secret, RequesterID, AttesterID, and HostAddress fields of L1RespondSecretTx. This could lead to a runtime panic due to a nil slice. The code should be updated to properly initialize data with the relevant fields before appending.
- var data []byte
+ data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret))Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx { | |
| var data []byte | |
| func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx { | |
| data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret)) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- go/host/l1/publisher.go (6 hunks)
Additional comments: 9
go/host/l1/publisher.go (9)
4-9: The import section is clean and well-organized, with no apparent unused imports.
28-38: The addition of the
importantContractAddressesmap and theimportantAddressesMutexis a good practice for thread-safe access to shared data. However, ensure that all read and write operations toimportantContractAddressesare properly guarded with this mutex to prevent data races.63-69: Initialization of the
importantContractAddressesmap and theimportantAddressesMutexis correct and follows Go's idiomatic approach.161-162: The
PublishSecretResponsemethod correctly uses a goroutine for the fire-and-forget approach. Ensure that the system can handle the asynchronous nature of this call without any adverse effects on the transaction lifecycle or error handling.164-187: The
ExtractObscuroRelevantTransactionsmethod is well-structured and cleanly separates the different transaction types. Ensure that theDecodeTxmethod correctly identifies and decodes all relevant transaction types.221-236: The
FetchLatestPeersListmethod is correctly fetching and decoding the list of host addresses. However, ensure that theDecodeHostAddressesResponsemethod handles any potential errors in the response format.251-252: Returning the filtered list of host addresses is correct. Ensure that the list is used appropriately and that the peer-to-peer communication mechanism can handle changes in the peer list.
254-258: The
GetImportantContractsmethod correctly uses a read lock to access the sharedimportantContractAddressesmap. This is a good practice for thread safety.260-301: The
ResyncImportantContractsmethod is correctly structured to fetch and update the important contracts. Ensure that theCallContractand decoding methods handle all possible errors and edge cases. Also, consider adding a mechanism to handle the case where the contract list changes while it is being fetched, to ensure consistency.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (1 hunks)
Additional comments: 1
go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (1)
- 3-16: The changes to the constants and the addition of new methods in the
mgmtcontractlibpackage indicate an expansion of the library's capabilities to handle important contract addresses. It is crucial to ensure that all references to these constants and methods are updated across the codebase to reflect the new functionality. Additionally, the use of//#nosecin line 9 suggests that a security scanner should ignore this line. Verify that this is indeed the desired behavior and that no security vulnerabilities are introduced by this method.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (8)
- go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (1 hunks)
- go/ethadapter/mgmtcontractlib/mgmt_contract_lib.go (4 hunks)
- integration/ethereummock/mgmt_contract_lib.go (1 hunks)
- integration/networktest/actions/l1/important_contracts.go (1 hunks)
- integration/networktest/env/testnet.go (2 hunks)
- integration/networktest/interfaces.go (3 hunks)
- integration/networktest/tests/bridge/important_contracts_test.go (1 hunks)
- integration/simulation/devnetwork/dev_network.go (1 hunks)
Files skipped from review due to trivial changes (2)
- integration/networktest/tests/bridge/important_contracts_test.go
- integration/simulation/devnetwork/dev_network.go
Additional comments: 13
go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (1)
- 3-16: The changes to the constants in the
mgmtcontractlibpackage reflect the addition of new functionality for managing important contract addresses. The new constantsGetImportantContractKeysMethod,SetImportantContractsMethod, andGetImportantAddressMethodare well-named and consistent with the existing naming conventions in the codebase. However, ensure that the actual method names in the smart contract match these constants to avoid runtime errors.integration/networktest/interfaces.go (2)
2-8: The import of
github.com/obscuronet/go-obscuro/go/walletis added, which is necessary for the newGetMCOwnerWalletmethod in theNetworkConnectorinterface. Ensure that this import is used elsewhere in the code, otherwise, it could be an unnecessary addition.11-19: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [11-27]
The
NetworkConnectorinterface is updated with a new methodGetMCOwnerWallet(). This method is expected to return a wallet that owns the management contract, which is a critical addition for network administration tasks. Ensure that all implementations of theNetworkConnectorinterface are updated to include this new method.integration/networktest/env/testnet.go (2)
4-12: The import of the
walletpackage is added to support the newGetMCOwnerWalletmethod. Ensure that this package is used elsewhere in the code to avoid importing it unnecessarily.130-131: The
GetMCOwnerWalletmethod is stubbed to returnniland an error, indicating that it's not supported in the testnet connector environment. This is a good practice for methods that are not applicable in certain contexts, as it clearly communicates to the caller that the operation is invalid. However, ensure that callers of this method are prepared to handle this error appropriately.integration/networktest/actions/l1/important_contracts.go (1)
- 1-15: The imports seem appropriate for the functionality described. However, it's important to ensure that all these packages are used in the code to avoid unnecessary imports, which can lead to increased compilation time and binary size.
go/ethadapter/mgmtcontractlib/mgmt_contract_lib.go (7)
28-48: The interface
MgmtContractLibhas been updated with new methods for managing important contract addresses. Ensure that all implementations of this interface are updated accordingly to avoid any runtime errors due to missing method implementations.201-207: The method
GetHostAddressesMsgis correctly implemented to create a call message for retrieving host addresses. However, ensure that the error message provides enough context for debugging in case of failure.209-225: The method
DecodeHostAddressesResponseunpacks the response from a call to get host addresses. It's good to see error handling and validation of the response structure. Ensure that the error messages are descriptive enough to aid in troubleshooting.253-259: The method
SetImportantContractMsgis implemented to create a call message for setting an important contract address. Ensure that the key and address parameters are validated before being used to avoid any potential issues with invalid data.287-311: The method
DecodeImportantAddressResponsecorrectly unpacks the response from a call to get an important address. The error handling is consistent with the rest of the code. Ensure that the error messages are descriptive enough to aid in troubleshooting.403-431: The method
unpackSetImportantContractsTxunpacks the transaction data for setting important contracts. It's good to see that errors are being returned rather than panicking. Ensure that the rest of the codebase follows this pattern for error handling.433-435: The helper function
base64EncodeToStringis a utility for encoding bytes to a base64 string. This is a common operation and the implementation looks correct.
| func (s *setImportantContract) Verify(ctx context.Context, network networktest.NetworkConnector) error { | ||
| cli, err := obsclient.Dial(network.ValidatorRPCAddress(0)) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to dial obsClient") | ||
| } | ||
| networkCfg, err := cli.GetConfig() | ||
| fmt.Println("networkCfg: ", networkCfg) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to get network config") | ||
| } | ||
|
|
||
| if networkCfg.ImportantContracts == nil || len(networkCfg.ImportantContracts) == 0 { | ||
| return errors.New("no important contracts set") | ||
| } | ||
| if len(networkCfg.ImportantContracts) > 1 { | ||
| return errors.New("more than one important contract set") | ||
| } | ||
| if addr, ok := networkCfg.ImportantContracts[s.contractKey]; !ok || addr != s.contractAddress { | ||
| return errors.New("important contract not set") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The Verify method checks if the important contract address has been set correctly. The error handling is consistent, and the logic checks for the existence and correctness of the important contract address. However, there are a couple of points to consider:
- The check on lines 100-105 assumes that there should be exactly one important contract set. If the system is expected to handle multiple important contracts, this logic may need to be revised.
- The print statement on line 95 should be removed or replaced with a proper logging statement, as print statements are not suitable for production code due to their synchronous nature and lack of control over the output format and destination.
BedrockSquirrel
force-pushed
the
matt/load-contract-addresses
branch
from
cceced0 to
423eda1
Compare
BedrockSquirrel
force-pushed
the
matt/load-contract-addresses
branch
from
423eda1 to
05a9a3d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 13
Configuration used: CodeRabbit UI
Files selected for processing (14)
- go/common/host/services.go (2 hunks)
- go/common/query_types.go (1 hunks)
- go/ethadapter/l1_transaction.go (1 hunks)
- go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (1 hunks)
- go/ethadapter/mgmtcontractlib/mgmt_contract_lib.go (4 hunks)
- go/host/enclave/guardian.go (3 hunks)
- go/host/host.go (1 hunks)
- go/host/l1/publisher.go (6 hunks)
- integration/ethereummock/mgmt_contract_lib.go (1 hunks)
- integration/networktest/actions/l1/important_contracts.go (1 hunks)
- integration/networktest/env/testnet.go (2 hunks)
- integration/networktest/interfaces.go (2 hunks)
- integration/networktest/tests/bridge/important_contracts_test.go (1 hunks)
- integration/simulation/devnetwork/dev_network.go (1 hunks)
Files skipped from review due to trivial changes (1)
- integration/simulation/devnetwork/dev_network.go
Additional comments: 20
go/common/query_types.go (1)
- 89-92: The addition of the
ImportantContractsfield to theObscuroNetworkInfostruct is a significant change. It is important to ensure that all parts of the system that instantiate or use this struct are updated to handle this new field. This includes serialization and deserialization logic, as well as any database schema that storesObscuroNetworkInfoobjects. Additionally, consider thread safety if this struct is accessed concurrently.go/common/host/services.go (2)
101-108: The renaming of the functions to
ExtractObscuroRelevantTransactionsand the change in its return type to three slices of different transaction types is a significant change. Ensure that all parts of the codebase that interact with this function are updated to handle the new return types correctly. Additionally, the comment on line 107 suggests a potential issue with rollup transaction publication failure. It would be beneficial to address this concern as part of the code changes to ensure reliability in the rollup process.116-119: The addition of
GetImportantContractsandResyncImportantContractsmethods to theL1Publisherinterface is a critical update for managing important contract addresses. Ensure that the implementation of these methods properly handles synchronization issues, especially in a multi-threaded environment where race conditions could occur. It's also important to ensure that the cache invalidation strategy is robust to prevent stale data from being served.integration/networktest/interfaces.go (3)
4-10: The import of the
walletpackage is added to support the newGetMCOwnerWallet()method in theNetworkConnectorinterface. Ensure that this package is used elsewhere in the code to justify its inclusion.16-16: The comment has been updated to reflect that some methods may not be available for certain networks. This is a useful note for developers to understand the limitations of the interface based on the network context.
27-27: The
GetMCOwnerWallet()method has been added to theNetworkConnectorinterface. This method should return the wallet that owns the management contract, which is a critical piece of functionality for network administration. Ensure that all implementations of theNetworkConnectorinterface provide a valid implementation of this method.go/ethadapter/mgmtcontractlib/mgmt_contract_ABI.go (2)
3-16: The changes to the constants in the
mgmtcontractlibpackage reflect the addition of new functionality for managing important contract addresses. It's important to ensure that these new constants match the actual method identifiers in the smart contract ABI. If the method identifiers are incorrect, the calls to these methods will fail. Additionally, the use of//#nosecon line 9 suggests that there is a security-related exception being made. This should be reviewed to ensure that it is justified and that any potential security implications are understood and accepted.16-16: The
MgmtContractABIvariable is updated to reflect the new ABI of theManagementContract. It's crucial to ensure that theManagementContracthas been updated accordingly and that all references to the ABI throughout the codebase are consistent with this change.integration/networktest/env/testnet.go (2)
4-17: The addition of the
errorspackage and thewalletimport fromgithub.com/obscuronet/go-obscuro/go/walletis necessary for the newGetMCOwnerWalletmethod. Ensure that these imports are used elsewhere in the file to avoid including unused imports.131-132: The
GetMCOwnerWalletmethod is stubbed to always return an error. If this is intentional to indicate that the functionality is not supported in the testnet environment, this is acceptable. However, if the method is expected to provide a validwallet.Walletin the future, a TODO comment or an issue should be created to track this incomplete feature.integration/networktest/tests/bridge/important_contracts_test.go (1)
- 1-23: The test function
TestImportantContractsLookupis well-structured and follows the standard Go conventions for writing tests. It uses thenetworktestframework to set up a local development network environment and then sets an important contract address using theSetImportantContractaction. The use ofnetworktest.TestOnlyRunsInIDEis a good practice to ensure that the test is only run in a development environment and not in a production-like environment. The test name is descriptive, and the steps are clear and concise.However, ensure that the
SetImportantContractaction is properly implemented and that it interacts correctly with the Obscuro network's components to set the important contract address as expected. Also, verify that theenv.LocalDevNetwork()function sets up the network environment correctly for this test case.go/ethadapter/l1_transaction.go (1)
- 36-39: The new
L1SetImportantContractsTxstruct has been introduced correctly with fieldsKeyandNewAddress. This struct is presumably used to represent transactions that set important contract addresses on the network. Ensure that the rest of the codebase is updated to handle this new transaction type where necessary.go/ethadapter/mgmtcontractlib/mgmt_contract_lib.go (2)
28-48: The interface
MgmtContractLibhas been updated with new methods for managing important contract addresses. This is a significant change and should be carefully reviewed to ensure that all implementations of this interface are updated accordingly. The removal of theGetHostAddressesandDecodeCallResponsemethods should also be verified to ensure that they are not used elsewhere in the codebase.403-431: The
unpackSetImportantContractsTxmethod has been added to handle the unpacking of transactions related to setting important contracts. This method correctly returns an error instead of panicking, which is a good practice. Ensure that all callers of this method handle the error appropriately.integration/networktest/actions/l1/important_contracts.go (1)
- 3-15: The imports are well-organized and grouped according to Go conventions: standard library, third-party packages, and internal packages. However, ensure that all imported packages are used within the file to avoid unnecessary imports.
go/host/l1/publisher.go (5)
- 4-9: The import of the
stopcontrolpackage is no longer used in the provided code hunk. If it's not used elsewhere in the file, it should be removed to keep the code clean and avoid unnecessary package dependencies.- "github.com/ten-protocol/go-ten/go/common/stopcontrol"
28-38: The addition of the
importantContractAddressesmap and theimportantAddressesMutexto thePublisherstruct is a good practice for thread-safe access to shared resources. This is especially important in a concurrent environment like a network service.161-187: The
ExtractObscuroRelevantTransactionsmethod has been updated to handle the new transaction typeL1SetImportantContractsTx. This is a good example of extending functionality while maintaining the structure of the existing code. However, ensure that the decoding logic withinmgmtContractLib.DecodeTxis robust and can handle new transaction types without affecting the existing ones.223-237: The
FetchLatestPeersListmethod seems to be correctly filtering out the publisher's own address and deduplicating the list of host addresses. This is important to prevent the node from trying to connect to itself and to reduce unnecessary network traffic.254-258: The
GetImportantContractsmethod provides a thread-safe way to access the important contract addresses. Using read-write locks (sync.RWMutex) is a good choice here since it allows multiple concurrent read accesses, which can improve performance when there are many read operations and relatively few writes.
| HostAddress string | ||
| } | ||
|
|
||
| type L1SetImportantContractsTx struct { | ||
| Key string | ||
| NewAddress gethcommon.Address | ||
| } | ||
|
|
||
| // Sign signs the payload with a given private key | ||
| func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx { | ||
| var data []byte |
There was a problem hiding this comment.
The Sign function for L1RespondSecretTx has been updated to include the HostAddress in the data that is signed. However, the initialization of the data slice is missing, which could lead to a runtime panic due to a nil slice dereference when append is called. The data slice should be initialized with an appropriate size before appending elements to it. Here's a proposed change:
- var data []byte
+ data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret))This will allocate a slice with enough capacity to hold all the bytes that will be appended, avoiding potential reallocations during the append operations.
Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| HostAddress string | |
| } | |
| type L1SetImportantContractsTx struct { | |
| Key string | |
| NewAddress gethcommon.Address | |
| } | |
| // Sign signs the payload with a given private key | |
| func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx { | |
| var data []byte | |
| HostAddress string | |
| } | |
| type L1SetImportantContractsTx struct { | |
| Key string | |
| NewAddress gethcommon.Address | |
| } | |
| // Sign signs the payload with a given private key | |
| func (l *L1RespondSecretTx) Sign(privateKey *ecdsa.PrivateKey) *L1RespondSecretTx { | |
| data := make([]byte, 0, len(l.AttesterID.Bytes()) + len(l.RequesterID.Bytes()) + len(l.HostAddress) + len(l.Secret)) |
| ManagementContractAddress: h.config.ManagementContractAddress, | ||
| L1StartHash: h.config.L1StartHash, | ||
|
|
||
| SequencerID: h.config.SequencerID, | ||
| MessageBusAddress: h.config.MessageBusAddress, | ||
| SequencerID: h.config.SequencerID, | ||
| MessageBusAddress: h.config.MessageBusAddress, | ||
| ImportantContracts: h.services.L1Publisher().GetImportantContracts(), | ||
| }, nil |
There was a problem hiding this comment.
The implementation of ObscuroConfig does not handle the potential error that could be returned by GetImportantContracts. This could lead to a runtime panic if GetImportantContracts encounters an error. It is important to handle this error to ensure the robustness of the code. Here is a suggested change:
func (h *host) ObscuroConfig() (*common.ObscuroNetworkInfo, error) {
importantContracts, err := h.services.L1Publisher().GetImportantContracts()
if err != nil {
return nil, fmt.Errorf("failed to get important contracts: %w", err)
}
return &common.ObscuroNetworkInfo{
ManagementContractAddress: h.config.ManagementContractAddress,
L1StartHash: h.config.L1StartHash,
SequencerID: h.config.SequencerID,
MessageBusAddress: h.config.MessageBusAddress,
ImportantContracts: importantContracts,
}, nil
}This change ensures that any errors encountered when retrieving important contracts are properly handled and reported, preventing potential runtime issues.
go/host/enclave/guardian.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err) | ||
| } | ||
| secretRespTxs := g.sl.L1Publisher().ExtractSecretResponses(nextBlock) | ||
| secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock) | ||
| if err != nil { | ||
| return fmt.Errorf("could not extract secret responses from block=%s - %w", nextBlock.Hash(), err) | ||
| } |
There was a problem hiding this comment.
The error handling for ExtractObscuroRelevantTransactions is incorrect. The error check is repeated and does not correspond to the actual call to ExtractObscuroRelevantTransactions. This could lead to a situation where an error from ExtractObscuroRelevantTransactions is not handled, potentially causing the guardian to miss important transactions.
- if err != nil {
- return fmt.Errorf("could not extract secret responses from block=%s - %w", nextBlock.Hash(), err)
- }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err != nil { | |
| return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err) | |
| } | |
| secretRespTxs := g.sl.L1Publisher().ExtractSecretResponses(nextBlock) | |
| secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock) | |
| if err != nil { | |
| return fmt.Errorf("could not extract secret responses from block=%s - %w", nextBlock.Hash(), err) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err) | |
| } | |
| secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock) |
| func (m *mockContractLib) GetHostAddressesMsg() (ethereum.CallMsg, error) { | ||
| return ethereum.CallMsg{}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) DecodeCallResponse([]byte) ([][]string, error) { | ||
| return [][]string{{""}}, nil | ||
| func (m *mockContractLib) DecodeHostAddressesResponse([]byte) ([]string, error) { | ||
| return []string{""}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) GetImportantContractKeysMsg() (ethereum.CallMsg, error) { | ||
| return ethereum.CallMsg{}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) DecodeImportantContractKeysResponse([]byte) ([]string, error) { | ||
| return []string{""}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) SetImportantContractMsg(string, gethcommon.Address) (ethereum.CallMsg, error) { | ||
| return ethereum.CallMsg{}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) GetImportantAddressCallMsg(string) (ethereum.CallMsg, error) { | ||
| return ethereum.CallMsg{}, nil | ||
| } | ||
|
|
||
| func (m *mockContractLib) DecodeImportantAddressResponse([]byte) (gethcommon.Address, error) { | ||
| return gethcommon.Address{}, nil | ||
| } |
There was a problem hiding this comment.
The mock implementations for the functions GetHostAddressesMsg, DecodeHostAddressesResponse, GetImportantContractKeysMsg, DecodeImportantContractKeysResponse, SetImportantContractMsg, GetImportantAddressCallMsg, and DecodeImportantAddressResponse are returning default values without any logic. If these functions are meant to simulate the behavior of their real counterparts, they should be implemented to return meaningful mock data. If they are intended to be stubs, it would be beneficial to add comments to clarify this to maintainers and other developers. Otherwise, it could lead to confusion or incorrect assumptions about the functionality of these methods.
For instance, DecodeHostAddressesResponse and DecodeImportantContractKeysResponse should decode the input bytes into the expected format, and GetImportantAddressCallMsg and SetImportantContractMsg should create a CallMsg that reflects the input parameters.
Here's an example of how you might implement DecodeHostAddressesResponse to return a mock response:
func (m *mockContractLib) DecodeHostAddressesResponse(_ []byte) ([]string, error) {
// Mock implementation: return a slice of mock addresses as strings.
mockAddresses := []string{"0xMockAddress1", "0xMockAddress2"}
return mockAddresses, nil
}| } | ||
|
|
||
| func (m *mockContractLib) DecodeImportantAddressResponse([]byte) (gethcommon.Address, error) { | ||
| return gethcommon.Address{}, nil | ||
| } | ||
|
|
||
| func decodeTx(tx *types.Transaction) ethadapter.L1Transaction { |
There was a problem hiding this comment.
The use of panic in the decodeTx function is not recommended for error handling in Go, as it can lead to abrupt termination of the program and is not a graceful way to handle errors. In a production environment, it's better to return an error and handle it at a higher level. Even in a mock context, it would be more appropriate to return an error that can be checked by the calling function. This would allow for better testing and debugging, as well as making the code more robust and maintainable.
Here's how you might refactor the decodeTx function to return an error instead of panicking:
func decodeTx(tx *types.Transaction) (ethadapter.L1Transaction, error) {
if len(tx.Data()) == 0 {
return nil, errors.New("data cannot be 0 in the mock implementation")
}
// ... (rest of the code)
if err := dec.Decode(t); err != nil {
return nil, err // return the error instead of panicking
}
return t, nil
}And then the calling code would need to handle this error appropriately.
|
|
||
| case InitializeSecretMethod: | ||
| return c.unpackInitSecretTx(tx, method, contractCallData) | ||
|
|
||
| case SetImportantContractsMethod: | ||
| tx, err := c.unpackSetImportantContractsTx(tx, method, contractCallData) | ||
| if err != nil { | ||
| c.logger.Warn("could not unpack set important contracts tx", log.ErrKey, err) | ||
| return nil | ||
| } | ||
| return tx | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
The DecodeTx method now includes a case for SetImportantContractsMethod. The error handling has been improved by logging a warning instead of panicking, which is a good practice to avoid service disruption. However, ensure that all possible error paths are handled correctly and that the system can recover from these errors gracefully.
// Replace the panic calls with proper error handling
func (c *contractLibImpl) DecodeTx(tx *types.Transaction) ethadapter.L1Transaction {
// ... existing code ...
method, err := c.contractABI.MethodById(tx.Data()[:methodBytesLen])
if err != nil {
c.logger.Warn("could not find method by ID", log.ErrKey, err)
return nil
}
// ... existing switch case ...
default:
c.logger.Warn("unknown method", "methodName", method.Name)
return nil
}
}| func (c *contractLibImpl) GetContractNamesMsg() (ethereum.CallMsg, error) { | ||
| data, err := c.contractABI.Pack(GetImportantContractKeysMethod) | ||
| if err != nil { | ||
| return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err) | ||
| } | ||
| return ethereum.CallMsg{To: c.addr, Data: data}, nil | ||
| } | ||
|
|
||
| func (c *contractLibImpl) DecodeContractNamesResponse(callResponse []byte) ([]string, error) { | ||
| unpackedResponse, err := c.contractABI.Unpack(GetImportantContractKeysMethod, callResponse) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not unpack call response. Cause: %w", err) | ||
| } | ||
|
|
||
| // We expect the response to be a list containing one element, that element is a list of address strings | ||
| if len(unpackedResponse) != 1 { | ||
| return nil, fmt.Errorf("unexpected number of results (%d) returned from call, response: %s", len(unpackedResponse), unpackedResponse) | ||
| } | ||
| contractNames, ok := unpackedResponse[0].([]string) | ||
| if !ok { | ||
| return nil, fmt.Errorf("could not convert element in call response to list of strings") | ||
| } | ||
|
|
||
| return contractNames, nil | ||
| } |
There was a problem hiding this comment.
The DecodeContractNamesResponse method appears to be a duplicate of the DecodeImportantContractKeysResponse method (lines 269-285). If this is not intentional and the functionality is indeed the same, one of the methods should be removed to avoid redundancy and potential confusion.
- func (c *contractLibImpl) DecodeContractNamesResponse(callResponse []byte) ([]string, error) {
- // ... existing code ...
- }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (c *contractLibImpl) GetContractNamesMsg() (ethereum.CallMsg, error) { | |
| data, err := c.contractABI.Pack(GetImportantContractKeysMethod) | |
| if err != nil { | |
| return ethereum.CallMsg{}, fmt.Errorf("could not pack the call data. Cause: %w", err) | |
| } | |
| return ethereum.CallMsg{To: c.addr, Data: data}, nil | |
| } | |
| func (c *contractLibImpl) DecodeContractNamesResponse(callResponse []byte) ([]string, error) { | |
| unpackedResponse, err := c.contractABI.Unpack(GetImportantContractKeysMethod, callResponse) | |
| if err != nil { | |
| return nil, fmt.Errorf("could not unpack call response. Cause: %w", err) | |
| } | |
| // We expect the response to be a list containing one element, that element is a list of address strings | |
| if len(unpackedResponse) != 1 { | |
| return nil, fmt.Errorf("unexpected number of results (%d) returned from call, response: %s", len(unpackedResponse), unpackedResponse) | |
| } | |
| contractNames, ok := unpackedResponse[0].([]string) | |
| if !ok { | |
| return nil, fmt.Errorf("could not convert element in call response to list of strings") | |
| } | |
| return contractNames, nil | |
| } |
| func (s *setImportantContract) Run(ctx context.Context, network networktest.NetworkConnector) (context.Context, error) { | ||
| obsClient, err := obsclient.Dial(network.ValidatorRPCAddress(0)) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to dial obsClient") | ||
| } | ||
|
|
||
| networkCfg, err := obsClient.GetConfig() | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to get network config") | ||
| } | ||
|
|
||
| l1Client, err := network.GetL1Client() | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to get L1 client") | ||
| } | ||
|
|
||
| mgmtContract := mgmtcontractlib.NewMgmtContractLib(&networkCfg.ManagementContractAddress, testlog.Logger()) | ||
|
|
||
| msg, err := mgmtContract.SetImportantContractMsg(s.contractKey, s.contractAddress) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to create SetImportantContractMsg") | ||
| } | ||
|
|
||
| txData := &types.LegacyTx{ | ||
| To: &networkCfg.ManagementContractAddress, | ||
| Data: msg.Data, | ||
| } | ||
| mcOwner, err := network.GetMCOwnerWallet() | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to get MC owner wallet") | ||
| } | ||
| // !! Important note !! | ||
| // The ownerOnly check in the contract doesn't like the gas estimate in here, to test you may need to hardcode a | ||
| // the gas value when the estimate errors | ||
| tx, err := l1Client.PrepareTransactionToSend(txData, networkCfg.ManagementContractAddress, mcOwner.GetNonceAndIncrement()) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to prepare tx") | ||
| } | ||
| signedTx, err := mcOwner.SignTransaction(tx) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to sign tx") | ||
| } | ||
| err = l1Client.SendTransaction(signedTx) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to send tx") | ||
| } | ||
|
|
||
| // wait for tx to be mined | ||
| return ctx, retry.Do(func() error { | ||
| receipt, err := l1Client.TransactionReceipt(signedTx.Hash()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if receipt.Status != types.ReceiptStatusSuccessful { | ||
| return retry.FailFast(errors.New("tx failed")) | ||
| } | ||
| return nil | ||
| }, retry.NewTimeoutStrategy(15*time.Second, 1*time.Second)) |
There was a problem hiding this comment.
The Run method is well-structured and follows a clear sequence of steps to set an important contract. Error handling is consistent, wrapping errors to provide context. However, the comment on lines 61-62 suggests a potential issue with gas estimation that might require a hardcoded gas value for testing. This should be addressed in a more robust way, possibly by allowing a configurable gas limit or improving the gas estimation logic.
// Potential improvement for gas estimation issue
// Allow passing a custom gas limit as a parameter to the action
type setImportantContract struct {
contractKey string
contractAddress common.Address
gasLimit uint64 // Add a gasLimit field
}
// ...
// Use the custom gas limit if provided
if s.gasLimit > 0 {
txData.Gas = s.gasLimit
} else {
// Perform gas estimation logic
}| // ResyncImportantContracts will fetch the latest important contracts from the management contract and update the cached map | ||
| // Note: this should be run in a goroutine as it makes L1 transactions in series and will block. | ||
| // Cache is not overwritten until it completes. | ||
| func (p *Publisher) ResyncImportantContracts() error { | ||
| getKeysCallMsg, err := p.mgmtContractLib.GetImportantContractKeysMsg() | ||
| if err != nil { | ||
| return fmt.Errorf("could not build callMsg for important contracts: %w", err) | ||
| } | ||
| keysResp, err := p.ethClient.CallContract(getKeysCallMsg) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch important contracts: %w", err) | ||
| } | ||
|
|
||
| importantContracts, err := p.mgmtContractLib.DecodeImportantContractKeysResponse(keysResp) | ||
| if err != nil { | ||
| return fmt.Errorf("could not decode important contracts resp: %w", err) | ||
| } | ||
|
|
||
| contractsMap := make(map[string]gethcommon.Address) | ||
|
|
||
| for _, contract := range importantContracts { | ||
| getAddressCallMsg, err := p.mgmtContractLib.GetImportantAddressCallMsg(contract) | ||
| if err != nil { | ||
| return fmt.Errorf("could not build callMsg for important contract=%s: %w", contract, err) | ||
| } | ||
| addrResp, err := p.ethClient.CallContract(getAddressCallMsg) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch important contract=%s: %w", contract, err) | ||
| } | ||
| contractAddress, err := p.mgmtContractLib.DecodeImportantAddressResponse(addrResp) | ||
| if err != nil { | ||
| return fmt.Errorf("could not decode important contract=%s resp: %w", contract, err) | ||
| } | ||
| contractsMap[contract] = contractAddress | ||
| } | ||
|
|
||
| p.importantAddressesMutex.Lock() | ||
| defer p.importantAddressesMutex.Unlock() | ||
| p.importantContractAddresses = contractsMap | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The ResyncImportantContracts method is designed to update the cache of important contract addresses. It's good to see that the cache is not overwritten until the new data is fully fetched and processed, which prevents inconsistent states. However, consider adding error handling for the case when the ethClient.CallContract call fails due to network issues. Implementing a retry mechanism could be beneficial here.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (4)
- go/host/enclave/guardian.go (3 hunks)
- integration/networktest/actions/l1/important_contracts.go (1 hunks)
- integration/networktest/env/testnet.go (2 hunks)
- integration/networktest/tests/bridge/important_contracts_test.go (1 hunks)
Additional comments: 5
integration/networktest/tests/bridge/important_contracts_test.go (1)
- 1-23:
The test function
TestImportantContractsLookupappears to be correctly set up for testing the setting of an important contract address in a local development network. Ensure that the address provided togethcommon.HexToAddressis valid and that theSetImportantContractaction behaves as expected when provided with this address.integration/networktest/env/testnet.go (3)
4-17: The import of the "github.com/ten-protocol/go-ten/go/wallet" package and the "errors" package is correctly added to support the new
GetMCOwnerWalletmethod and error handling. Ensure that these packages are used elsewhere in the code to avoid importing them unnecessarily.131-132: The
GetMCOwnerWalletmethod is stubbed to always return an error. If this is intentional to indicate that the functionality is not supported in the testnet environment, this is acceptable. However, if the method is expected to provide a valid wallet in the future, a TODO comment or an issue should be created to track this incomplete feature.127-128: The
AllocateFaucetFundsWithWalletmethod has been updated to return an error, which is a good practice for error handling. Ensure that all callers of this method handle the returned error appropriately.integration/networktest/actions/l1/important_contracts.go (1)
- 1-20: The new
setImportantContractstruct andSetImportantContractfunction are well-defined and follow Go naming conventions. The struct fields are unexported, which is good for encapsulation.
| if err != nil { | ||
| return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err) | ||
| } | ||
| secretRespTxs := g.sl.L1Publisher().ExtractSecretResponses(nextBlock) | ||
| if err != nil { | ||
| return fmt.Errorf("could not extract secret responses from block=%s - %w", nextBlock.Hash(), err) | ||
| } | ||
| secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock) | ||
| for _, scrt := range secretRespTxs { | ||
| if scrt.RequesterID.Hex() == g.hostData.ID.Hex() { | ||
| err = g.enclaveClient.InitEnclave(scrt.Secret) |
There was a problem hiding this comment.
The error handling for g.enclaveClient.InitEnclave(scrt.Secret) is missing. If the initialization fails, the loop should continue to the next secret response or exit the retry loop if there are no more responses. The current implementation will exit the retry loop even if there are more secret responses to try, which could lead to missed opportunities for successful enclave initialization.
- err = g.enclaveClient.InitEnclave(scrt.Secret)
+ if err := g.enclaveClient.InitEnclave(scrt.Secret); err != nil {
+ g.logger.Error("Could not initialize enclave with received secret response", log.ErrKey, err)
+ continue // try the next secret response in the block if there are more
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err != nil { | |
| return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err) | |
| } | |
| secretRespTxs := g.sl.L1Publisher().ExtractSecretResponses(nextBlock) | |
| if err != nil { | |
| return fmt.Errorf("could not extract secret responses from block=%s - %w", nextBlock.Hash(), err) | |
| } | |
| secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock) | |
| for _, scrt := range secretRespTxs { | |
| if scrt.RequesterID.Hex() == g.hostData.ID.Hex() { | |
| err = g.enclaveClient.InitEnclave(scrt.Secret) | |
| if err != nil { | |
| return fmt.Errorf("next block after block=%s not found - %w", awaitFromBlock, err) | |
| } | |
| secretRespTxs, _, _ := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(nextBlock) | |
| for _, scrt := range secretRespTxs { | |
| if scrt.RequesterID.Hex() == g.hostData.ID.Hex() { | |
| if err := g.enclaveClient.InitEnclave(scrt.Secret); err != nil { | |
| g.logger.Error("Could not initialize enclave with received secret response", log.ErrKey, err) | |
| continue // try the next secret response in the block if there are more | |
| } |
| func (s *setImportantContract) Run(ctx context.Context, network networktest.NetworkConnector) (context.Context, error) { | ||
| obsClient, err := obsclient.Dial(network.ValidatorRPCAddress(0)) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to dial obsClient") | ||
| } | ||
|
|
||
| networkCfg, err := obsClient.GetConfig() | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to get network config") | ||
| } | ||
|
|
||
| l1Client, err := network.GetL1Client() | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to get L1 client") | ||
| } | ||
|
|
||
| mgmtContract := mgmtcontractlib.NewMgmtContractLib(&networkCfg.ManagementContractAddress, testlog.Logger()) | ||
|
|
||
| msg, err := mgmtContract.SetImportantContractMsg(s.contractKey, s.contractAddress) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to create SetImportantContractMsg") | ||
| } | ||
|
|
||
| txData := &types.LegacyTx{ | ||
| To: &networkCfg.ManagementContractAddress, | ||
| Data: msg.Data, | ||
| } | ||
| mcOwner, err := network.GetMCOwnerWallet() | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to get MC owner wallet") | ||
| } | ||
| // !! Important note !! | ||
| // The ownerOnly check in the contract doesn't like the gas estimate in here, to test you may need to hardcode a | ||
| // the gas value when the estimate errors | ||
| tx, err := l1Client.PrepareTransactionToSend(txData, networkCfg.ManagementContractAddress, mcOwner.GetNonceAndIncrement()) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to prepare tx") | ||
| } | ||
| signedTx, err := mcOwner.SignTransaction(tx) | ||
| if err != nil { | ||
| return ctx, errors.Wrap(err, "failed to sign tx") | ||
| } | ||
| err = l1Client.SendTransaction(signedTx) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to send tx") | ||
| } | ||
|
|
||
| // wait for tx to be mined | ||
| return ctx, retry.Do(func() error { | ||
| receipt, err := l1Client.TransactionReceipt(signedTx.Hash()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if receipt.Status != types.ReceiptStatusSuccessful { | ||
| return retry.FailFast(errors.New("tx failed")) | ||
| } | ||
| return nil | ||
| }, retry.NewTimeoutStrategy(15*time.Second, 1*time.Second)) | ||
| } |
There was a problem hiding this comment.
The Run method is well-structured and follows a clear sequence of steps to set an important contract. However, the previous comment about the potential issue with gas estimation on lines 61-62 should be addressed. Consider allowing a configurable gas limit or improving the gas estimation logic.
type setImportantContract struct {
contractKey string
contractAddress common.Address
+ gasLimit uint64 // Optional: Add a gasLimit field to allow custom gas limit
}
...
+ // Use the custom gas limit if provided
+ if s.gasLimit > 0 {
+ txData.Gas = s.gasLimit
+ } else {
+ // Perform gas estimation logic
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (s *setImportantContract) Run(ctx context.Context, network networktest.NetworkConnector) (context.Context, error) { | |
| obsClient, err := obsclient.Dial(network.ValidatorRPCAddress(0)) | |
| if err != nil { | |
| return ctx, errors.Wrap(err, "failed to dial obsClient") | |
| } | |
| networkCfg, err := obsClient.GetConfig() | |
| if err != nil { | |
| return ctx, errors.Wrap(err, "failed to get network config") | |
| } | |
| l1Client, err := network.GetL1Client() | |
| if err != nil { | |
| return ctx, errors.Wrap(err, "failed to get L1 client") | |
| } | |
| mgmtContract := mgmtcontractlib.NewMgmtContractLib(&networkCfg.ManagementContractAddress, testlog.Logger()) | |
| msg, err := mgmtContract.SetImportantContractMsg(s.contractKey, s.contractAddress) | |
| if err != nil { | |
| return ctx, errors.Wrap(err, "failed to create SetImportantContractMsg") | |
| } | |
| txData := &types.LegacyTx{ | |
| To: &networkCfg.ManagementContractAddress, | |
| Data: msg.Data, | |
| } | |
| mcOwner, err := network.GetMCOwnerWallet() | |
| if err != nil { | |
| return ctx, errors.Wrap(err, "failed to get MC owner wallet") | |
| } | |
| // !! Important note !! | |
| // The ownerOnly check in the contract doesn't like the gas estimate in here, to test you may need to hardcode a | |
| // the gas value when the estimate errors | |
| tx, err := l1Client.PrepareTransactionToSend(txData, networkCfg.ManagementContractAddress, mcOwner.GetNonceAndIncrement()) | |
| if err != nil { | |
| return ctx, errors.Wrap(err, "failed to prepare tx") | |
| } | |
| signedTx, err := mcOwner.SignTransaction(tx) | |
| if err != nil { | |
| return ctx, errors.Wrap(err, "failed to sign tx") | |
| } | |
| err = l1Client.SendTransaction(signedTx) | |
| if err != nil { | |
| return nil, errors.Wrap(err, "failed to send tx") | |
| } | |
| // wait for tx to be mined | |
| return ctx, retry.Do(func() error { | |
| receipt, err := l1Client.TransactionReceipt(signedTx.Hash()) | |
| if err != nil { | |
| return err | |
| } | |
| if receipt.Status != types.ReceiptStatusSuccessful { | |
| return retry.FailFast(errors.New("tx failed")) | |
| } | |
| return nil | |
| }, retry.NewTimeoutStrategy(15*time.Second, 1*time.Second)) | |
| } | |
| type setImportantContract struct { | |
| contractKey string | |
| contractAddress common.Address | |
| gasLimit uint64 // Optional: Add a gasLimit field to allow custom gas limit | |
| } | |
| ... | |
| // Use the custom gas limit if provided | |
| if s.gasLimit > 0 { | |
| txData.Gas = s.gasLimit | |
| } else { | |
| // Perform gas estimation logic | |
| } |
| return ctx, retry.Do(func() error { | ||
| receipt, err := l1Client.TransactionReceipt(signedTx.Hash()) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if receipt.Status != types.ReceiptStatusSuccessful { | ||
| return retry.FailFast(errors.New("tx failed")) | ||
| } | ||
| return nil | ||
| }, retry.NewTimeoutStrategy(15*time.Second, 1*time.Second)) |
There was a problem hiding this comment.
The retry logic for waiting for the transaction to be mined is a good practice. However, the previous comment suggested making the timeout and interval configurable. This would allow the method to adapt to different network conditions.
type setImportantContract struct {
contractKey string
contractAddress common.Address
+ retryTimeout time.Duration // Optional: Add fields for retry strategy configuration
+ retryInterval time.Duration
}
...
+ // Use the custom retry strategy if provided
+ retryStrategy := retry.NewTimeoutStrategy(s.retryTimeout, s.retryInterval)
+ if s.retryTimeout == 0 || s.retryInterval == 0 {
+ retryStrategy = retry.NewTimeoutStrategy(15*time.Second, 1*time.Second) // Default values
+ }
...
- }, retry.NewTimeoutStrategy(15*time.Second, 1*time.Second))
+ }, retryStrategy)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| return ctx, retry.Do(func() error { | |
| receipt, err := l1Client.TransactionReceipt(signedTx.Hash()) | |
| if err != nil { | |
| return err | |
| } | |
| if receipt.Status != types.ReceiptStatusSuccessful { | |
| return retry.FailFast(errors.New("tx failed")) | |
| } | |
| return nil | |
| }, retry.NewTimeoutStrategy(15*time.Second, 1*time.Second)) | |
| return ctx, retry.Do(func() error { | |
| receipt, err := l1Client.TransactionReceipt(signedTx.Hash()) | |
| if err != nil { | |
| return err | |
| } | |
| if receipt.Status != types.ReceiptStatusSuccessful { | |
| return retry.FailFast(errors.New("tx failed")) | |
| } | |
| return nil | |
| }, retryStrategy) |
| func (g *Guardian) processL1BlockTransactions(block *common.L1Block) { | ||
| // if there are any secret responses in the block we should refresh our P2P list to re-sync with the network | ||
| respTxs := g.sl.L1Publisher().ExtractSecretResponses(block) | ||
| if len(respTxs) > 0 { | ||
| secretRespTxs, rollupTxs, contractAddressTxs := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(block) | ||
| if len(secretRespTxs) > 0 { | ||
| // new peers may have been granted access to the network, notify p2p service to refresh its peer list | ||
| go g.sl.P2P().RefreshPeerList() | ||
| } |
There was a problem hiding this comment.
The asynchronous call to g.sl.P2P().RefreshPeerList() is not handling potential errors. It is important to handle errors even in goroutines to ensure that the system can react appropriately, such as retrying the operation or alerting an operator.
- go g.sl.P2P().RefreshPeerList()
+ go func() {
+ if err := g.sl.P2P().RefreshPeerList(); err != nil {
+ g.logger.Error("Failed to refresh P2P peer list", log.ErrKey, err)
+ }
+ }()Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (g *Guardian) processL1BlockTransactions(block *common.L1Block) { | |
| // if there are any secret responses in the block we should refresh our P2P list to re-sync with the network | |
| respTxs := g.sl.L1Publisher().ExtractSecretResponses(block) | |
| if len(respTxs) > 0 { | |
| secretRespTxs, rollupTxs, contractAddressTxs := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(block) | |
| if len(secretRespTxs) > 0 { | |
| // new peers may have been granted access to the network, notify p2p service to refresh its peer list | |
| go g.sl.P2P().RefreshPeerList() | |
| } | |
| func (g *Guardian) processL1BlockTransactions(block *common.L1Block) { | |
| // if there are any secret responses in the block we should refresh our P2P list to re-sync with the network | |
| secretRespTxs, rollupTxs, contractAddressTxs := g.sl.L1Publisher().ExtractObscuroRelevantTransactions(block) | |
| if len(secretRespTxs) > 0 { | |
| // new peers may have been granted access to the network, notify p2p service to refresh its peer list | |
| go func() { | |
| if err := g.sl.P2P().RefreshPeerList(); err != nil { | |
| g.logger.Error("Failed to refresh P2P peer list", log.ErrKey, err) | |
| } | |
| }() | |
| } |
| func (s *setImportantContract) Verify(_ context.Context, network networktest.NetworkConnector) error { | ||
| cli, err := obsclient.Dial(network.ValidatorRPCAddress(0)) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to dial obsClient") | ||
| } | ||
| networkCfg, err := cli.GetConfig() | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to get network config") | ||
| } | ||
|
|
||
| if networkCfg.ImportantContracts == nil || len(networkCfg.ImportantContracts) == 0 { | ||
| return errors.New("no important contracts set") | ||
| } | ||
| if addr, ok := networkCfg.ImportantContracts[s.contractKey]; !ok || addr != s.contractAddress { | ||
| return errors.New("important contract not set") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The Verify method correctly checks if the important contract has been set as expected. However, the previous comment suggested adding more detailed logging or error information to help with debugging in case of failures.
if networkCfg.ImportantContracts == nil || len(networkCfg.ImportantContracts) == 0 {
- return errors.New("no important contracts set")
+ return errors.Errorf("no important contracts set in network config")
}
if addr, ok := networkCfg.ImportantContracts[s.contractKey]; !ok {
- return errors.New("important contract not set")
+ return errors.Errorf("important contract with key %s not found", s.contractKey)
} else if addr != s.contractAddress {
- return errors.New("important contract not set")
+ return errors.Errorf("important contract address mismatch: expected %s, got %s", s.contractAddress.Hex(), addr.Hex())
}Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func (s *setImportantContract) Verify(_ context.Context, network networktest.NetworkConnector) error { | |
| cli, err := obsclient.Dial(network.ValidatorRPCAddress(0)) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to dial obsClient") | |
| } | |
| networkCfg, err := cli.GetConfig() | |
| if err != nil { | |
| return errors.Wrap(err, "failed to get network config") | |
| } | |
| if networkCfg.ImportantContracts == nil || len(networkCfg.ImportantContracts) == 0 { | |
| return errors.New("no important contracts set") | |
| } | |
| if addr, ok := networkCfg.ImportantContracts[s.contractKey]; !ok || addr != s.contractAddress { | |
| return errors.New("important contract not set") | |
| } | |
| return nil | |
| func (s *setImportantContract) Verify(_ context.Context, network networktest.NetworkConnector) error { | |
| cli, err := obsclient.Dial(network.ValidatorRPCAddress(0)) | |
| if err != nil { | |
| return errors.Wrap(err, "failed to dial obsClient") | |
| } | |
| networkCfg, err := cli.GetConfig() | |
| if err != nil { | |
| return errors.Wrap(err, "failed to get network config") | |
| } | |
| if networkCfg.ImportantContracts == nil || len(networkCfg.ImportantContracts) == 0 { | |
| return errors.Errorf("no important contracts set in network config") | |
| } | |
| if addr, ok := networkCfg.ImportantContracts[s.contractKey]; !ok { | |
| return errors.Errorf("important contract with key %s not found", s.contractKey) | |
| } else if addr != s.contractAddress { | |
| return errors.Errorf("important contract address mismatch: expected %s, got %s", s.contractAddress.Hex(), addr.Hex()) | |
| } | |
| return nil |
| if len(contractAddressTxs) > 0 { | ||
| go func() { | ||
| err := g.sl.L1Publisher().ResyncImportantContracts() | ||
| if err != nil { | ||
| g.logger.Error("Could not resync important contracts", log.ErrKey, err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The previous comment about error handling within the goroutine for ResyncImportantContracts is still valid. There should be a retry mechanism or a way to alert the system that the resync did not succeed. This is crucial for maintaining consistency and ensuring that the system remains up-to-date.
- go func() {
- err := g.sl.L1Publisher().ResyncImportantContracts()
+ go retry.Do(func() error {
+ if err := g.sl.L1Publisher().ResyncImportantContracts(); err != nil {
+ g.logger.Error("Could not resync important contracts", log.ErrKey, err)
+ return err // return error to trigger a retry
+ }
+ return nil // success, no need to retry
+ }, retry.NewExponentialBackoffStrategy(5, 100*time.Millisecond, 2*time.Second))Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if len(contractAddressTxs) > 0 { | |
| go func() { | |
| err := g.sl.L1Publisher().ResyncImportantContracts() | |
| if err != nil { | |
| g.logger.Error("Could not resync important contracts", log.ErrKey, err) | |
| } | |
| }() | |
| if len(contractAddressTxs) > 0 { | |
| go retry.Do(func() error { | |
| if err := g.sl.L1Publisher().ResyncImportantContracts(); err != nil { | |
| g.logger.Error("Could not resync important contracts", log.ErrKey, err) | |
| return err // return error to trigger a retry | |
| } | |
| return nil // success, no need to retry | |
| }, retry.NewExponentialBackoffStrategy(5, 100*time.Millisecond, 2*time.Second)) |
| GetHostAddressesMethod = "GetHostAddresses" | ||
| GetImportantContractKeysMethod = "GetImportantContractKeys" | ||
| SetImportantContractsMethod = "SetImportantContractAddress" | ||
| GetImportantAddressMethod = "importantContractAddresses" |
There was a problem hiding this comment.
is this lower case intentional?
There was a problem hiding this comment.
Yeah, that's the method that solidity creates magically for the public field
| var secretRespTxs []*ethadapter.L1RespondSecretTx | ||
| var rollupTxs []*ethadapter.L1RollupTx | ||
| var contractAddressTxs []*ethadapter.L1SetImportantContractsTx |
There was a problem hiding this comment.
something for another PR, maybe , but we need to start using events here instead of rummaging through transactions.
Let's add a ticket/todo
There was a problem hiding this comment.
yeah good shout, I'll add a todo comment and ticket
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- go/host/l1/publisher.go (6 hunks)
Additional comments: 5
go/host/l1/publisher.go (5)
4-9:
Verify that all new imports are utilized and remove any that are not needed to avoid cluttering the namespace.28-38:
The addition ofimportantContractAddressesandimportantAddressesMutexis a good practice for managing concurrent access to shared resources.63-69:
The constructor correctly initializes the newimportantContractAddressesmap andimportantAddressesMutex.161-189:
TheExtractObscuroRelevantTransactionsmethod has been updated to handle the newL1SetImportantContractsTxtransaction type. Ensure that the logic for extracting and processing these transactions is correct and that the method is used appropriately in the codebase.223-240: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [225-253]
The
FetchLatestPeersListmethod has been updated to use a different method to retrieve host addresses. Ensure that the new logic correctly filters out duplicate and own addresses.
| func (p *Publisher) Start() error { | ||
| go func() { | ||
| // Do an initial read of important contract addresses when service starts up | ||
| err := p.ResyncImportantContracts() | ||
| if err != nil { | ||
| p.logger.Error("Could not load important contract addresses", log.ErrKey, err) | ||
| } | ||
| }() | ||
| return nil |
There was a problem hiding this comment.
The initial read of important contract addresses during service startup is a good practice to ensure that the service has the necessary data to operate. However, consider implementing a retry mechanism or a fallback strategy if the initial read fails to make the service more resilient to transient startup issues.
| func (p *Publisher) GetImportantContracts() map[string]gethcommon.Address { | ||
| p.importantAddressesMutex.RLock() | ||
| defer p.importantAddressesMutex.RUnlock() | ||
| return p.importantContractAddresses | ||
| } | ||
|
|
||
| // ResyncImportantContracts will fetch the latest important contracts from the management contract and update the cached map | ||
| // Note: this should be run in a goroutine as it makes L1 transactions in series and will block. | ||
| // Cache is not overwritten until it completes. | ||
| func (p *Publisher) ResyncImportantContracts() error { | ||
| getKeysCallMsg, err := p.mgmtContractLib.GetImportantContractKeysMsg() | ||
| if err != nil { | ||
| return fmt.Errorf("could not build callMsg for important contracts: %w", err) | ||
| } | ||
| keysResp, err := p.ethClient.CallContract(getKeysCallMsg) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch important contracts: %w", err) | ||
| } | ||
|
|
||
| importantContracts, err := p.mgmtContractLib.DecodeImportantContractKeysResponse(keysResp) | ||
| if err != nil { | ||
| return fmt.Errorf("could not decode important contracts resp: %w", err) | ||
| } | ||
|
|
||
| contractsMap := make(map[string]gethcommon.Address) | ||
|
|
||
| for _, contract := range importantContracts { | ||
| getAddressCallMsg, err := p.mgmtContractLib.GetImportantAddressCallMsg(contract) | ||
| if err != nil { | ||
| return fmt.Errorf("could not build callMsg for important contract=%s: %w", contract, err) | ||
| } | ||
| addrResp, err := p.ethClient.CallContract(getAddressCallMsg) | ||
| if err != nil { | ||
| return fmt.Errorf("could not fetch important contract=%s: %w", contract, err) | ||
| } | ||
| contractAddress, err := p.mgmtContractLib.DecodeImportantAddressResponse(addrResp) | ||
| if err != nil { | ||
| return fmt.Errorf("could not decode important contract=%s resp: %w", contract, err) | ||
| } | ||
| contractsMap[contract] = contractAddress | ||
| } | ||
|
|
||
| p.importantAddressesMutex.Lock() | ||
| defer p.importantAddressesMutex.Unlock() | ||
| p.importantContractAddresses = contractsMap | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The ResyncImportantContracts method is designed to update the cache of important contract addresses. It's good to see that the cache is not overwritten until the new data is fully fetched and processed, which prevents inconsistent states. However, consider adding error handling for the case when the ethClient.CallContract call fails due to network issues. Implementing a retry mechanism could be beneficial here.
There was a problem hiding this comment.
Should the mgm contract sol contract be updated too ?
| if len(contractAddressTxs) > 0 { | ||
| go func() { | ||
| err := g.sl.L1Publisher().ResyncImportantContracts() | ||
| if err != nil { | ||
| g.logger.Error("Could not resync important contracts", log.ErrKey, err) |
There was a problem hiding this comment.
This kinda breaks the security of the block ingestion, if we go back and do an eth_call someone malicious can interject. Should follow the same pattern of decoding the tx that it's known for a fact is "safe" ?
| // ResyncImportantContracts will fetch the latest important contracts from the management contract and update the cached map | ||
| // Note: this should be run in a goroutine as it makes L1 transactions in series and will block. | ||
| // Cache is not overwritten until it completes. | ||
| func (p *Publisher) ResyncImportantContracts() error { |
There was a problem hiding this comment.
Should we rely on eth_call instead of fetching the data over the minted block txt ?
|
Just for record so it doesn't look like I'm ignoring those comments haha, had a chat with Pedro:
|
Why this change is needed
For the near term we want to make important network contract addresses to be available. This is especially useful for short-lived testnets etc.
The data is published to the L1 management contract, this PR has node hosts monitor that contract and serve the addresses that are stored in it.
What changes were made as part of this PR
Serve the cached addresses with the network info RPC method.
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks